Sync stable/mi to main after release v0.24.0-wso2v92 - #556
Open
wso2-integration-bot wants to merge 55 commits into
Open
Sync stable/mi to main after release v0.24.0-wso2v92#556wso2-integration-bot wants to merge 55 commits into
wso2-integration-bot wants to merge 55 commits into
Conversation
Support the startOnLoad attribute for tasks in MI 4.1.0
Fix issues in the MI Extension
…erver # Conflicts: # org.eclipse.lemminx/src/test/resources/synapse/directorytree.builder/generated-directory-tree.json # org.eclipse.lemminx/src/test/resources/synapse/directorytree.builder/generated-project-explorer.json
Add MCP Inbound server implementation
Fix MCP tool response parsing issue
Update LS version
Fix issues in the MI Extension
Two validation gaps that let semantically-wrong but syntactically-valid
Synapse configs reach runtime unflagged:
- SemanticExpressionValidator: warn when a logical operator (and/or) is an
unparenthesized operand of a comparison. In the Synapse expression grammar
and/or bind TIGHTER than the comparison operators, so `a <= 0 or b > 10`
parses as `a <= (0 or b) > 10`. The warning points at the and/or token and
recommends parentheses; the corrected `(a <= 0) or (b > 10)` form is a single
top-level logical expression and is never flagged (no false positives).
- SynapseDiagnosticsParticipant: the UndefinedVariable check only scanned
attribute values, so a reference in element text (e.g.
<q>{${vars.soqlQuery1}}</q>) was never flagged. Extend it to scan leaf
element text content, sharing the same detection logic; skip raw-code bodies
(script) and containers to avoid false positives.
Adds tests for both in ExpressionValidatorTest and
SynapseDiagnosticsParticipantTest.
MI Copilot validates in-memory (unsaved) generated code via the synapse/codeDiagnostic RPC. That handler parsed the code with the literal URI "temp", which fails SynapseExpressionValidator's activation gate (it only runs for documents whose URI is under src/main/wso2mi/artifacts). As a result every expression diagnostic -- operator-precedence warnings, syntax errors, unknown-function/arg-count errors -- was silently dropped for the agent, even though the editor's didOpen -> publishDiagnostics flow reported them correctly. - CodeDiagnosticRequest: add a fileName field (the extension already sends it alongside code; it was being discarded). - Utils.getDOMDocument(content, uri, resolver): new overload that assigns the given URI. The existing 2-arg overload now delegates with uri="temp", so no other caller changes behavior. - SynapseLanguageService.codeDiagnostic(): use the request's fileName as the document URI when present, falling back to "temp" otherwise. This also unblocks the other URI-gated checks on this path (cross-file reference validation and the 4.4.0+ hints via deriveProjectPath). Adds CodeDiagnosticFileNameTest covering the artifacts-path, parenthesized, and "temp" fallback cases end-to-end through the diagnostics pipeline.
An opening "${" with no matching "}" (e.g. expression="${payload.count > 0")
was silently accepted: SynapseExpressionValidator only validates a value that
both starts with "${" and ends with "}", so an unterminated expression is
treated as a plain string and the malformed ${} boundary is never checked.
Add an UnclosedExpression warning in SynapseDiagnosticsParticipant that scans
attribute values and leaf element text for a "${" with no matching "}". It runs
for any Synapse document (so both the editor's didOpen flow and the
synapse/codeDiagnostic path are covered) and skips raw-code (script) bodies.
The detector is false-positive-safe: the expression grammar has no {/} tokens
of its own (indexing uses [ ]), so the only braces inside ${...} are within
string literals -- the scan ignores those, so a valid expression such as
${concat('}', x)} is correctly recognized as closed.
Adds tests covering unclosed in attributes and text, the closed and {${...}}
forms, a brace inside a string literal, and the script-body exclusion.
Two related fixes for the agent's per-file validation via synapse/codeDiagnostic,
where cross-file reference checks were firing spuriously.
1) Opt-in skipCrossFileValidation flag (default off) on synapse/codeDiagnostic.
The agent validates a file right after writing it, before the sibling
artifacts it references exist, so cross-file checks (UnresolvedArtifactReference,
UnresolvedConfigKeyReference, DuplicateArtifactName, UnknownTemplateParameter,
CircularArtifactReference, ...) misfire. The flag lets the agent suppress only
those checks; the editor and the explicit "validate all" path never set it and
are unchanged.
- CodeDiagnosticRequest: add boolean skipCrossFileValidation (default false).
- SynapseDiagnosticsParticipant: a thread-confined flag (set/cleared by
codeDiagnostic around doDiagnostics). When set, the run skips
buildArtifactNameIndex (so the knownArtifacts-gated checks and the filesystem
scan are skipped) and the two cross-file checks not gated on that index
(validateCallTemplateParams, validateDuplicateArtifactName). The skip path
mutates no shared index state, so concurrent editor validations are
unaffected; within-file checks (schema, expression/precedence,
UndefinedVariable, ...) keep running.
- SynapseLanguageService.codeDiagnostic(): set the flag from the request and
clear it in a finally block.
2) Refresh the cached cross-file artifact index when project files change.
The index is cached per project with a ~5s TTL, so just after a file is
written/saved the cached index can omit it, wrongly flagging a sibling that
exists on disk as unresolved (affects the editor and "validate all" too).
- SynapseDiagnosticsParticipant: a static epoch counter and
invalidateArtifactIndexCache(); each cache entry records its build epoch and
is honored only while that epoch still matches, so a bump forces a rebuild on
the next run even within the TTL.
- XMLWorkspaceService.didChangeWatchedFiles and XMLTextDocumentService.didSave:
bump the epoch when a changed/saved file under src/main/wso2mi is seen,
covering external/agent writes (file watcher) and editor saves.
Tests: skip=true drops UnresolvedArtifactReference while keeping the within-file
UndefinedVariable; default (false) still flags it; the request flag defaults
false; and a stale-index reference resolves after the sibling is written and the
cache is invalidated.
…-and-uninitialized-vars Improve Synapse expression and cross-file diagnostics for MI Copilot
The didSave and didChangeWatchedFiles handlers invalidate the cross-file artifact index when a changed file's URI contains "src/main/wso2mi". LSP document URIs use forward slashes on every OS, so this already works on Windows, but normalize the URI's separators before the check so a backslash path would also match. This is a small defensive hardening, consistent with the separator-agnostic artifacts-path gate in SynapseExpressionValidator. Follow-up to #552.
Add support to the binds-to attribute in APIs
Update LS version
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR expands diagnostics with URI-aware requests, cross-file validation controls, artifact-index invalidation, variable and unclosed-expression checks, and operator-precedence warnings. It adds MCP server classification, inbound connector discovery, protocol filtering, and a new RPC. API generation accepts context and version inputs, while API and APIResource support Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java (1)
126-168: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPurge removed custom connectors before rebuilding the cache.
This refresh path only adds the current zip set. It never removes old entries from
connectorIdMap,inboundConnectors, or the temp JSON cache, so a connector deleted from the project can still be resolved bygetInboundConnectorSchema*()and reloaded on the next init. Clear the custom cache before repopulating it so all inbound-connector endpoints see the same refreshed state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java` around lines 126 - 168, The refresh logic in InboundConnectorHolder#getCustomInboundConnectors only appends new connectors and leaves stale entries behind in connectorIdMap, inboundConnectors, and the temp JSON cache. Before iterating over the current zip set, clear the custom inbound-connector state and rebuild it from scratch, then repopulate the JSON array so getInboundConnectorSchema* and the init path all reflect the same refreshed set. Keep the existing extraction/import flow, but make sure removed connectors are no longer resolvable after a refresh.org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/RestApiAdmin.java (1)
123-147: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn an error payload for unsupported modes too.
These catch blocks now avoid
null, but an unknown or blankmodestill falls through toreturn nullon Line 147.SynapseLanguageService.generateAPI(...)forwards this result directly, so callers still need a null special case.Suggested fix
if (CREATE_FROM_SWAGGER.equalsIgnoreCase(mode)) { try { return createAPIFromSwagger(apiName, sourcePath, publishSwaggerPath); } catch (JsonProcessingException e) { LOGGER.log(Level.SEVERE, "Exception occurred while creating API from Swagger", e); return new GenerateAPIResponse(null, null, "Exception occurred while creating API from Swagger: " + e.getMessage()); } } else if (CREATE_FROM_WSDL.equalsIgnoreCase(mode)) { try { return createAPIFromWSDL(apiName, endpoint, sourcePath); } catch (SOAPToRESTException e) { LOGGER.log(Level.SEVERE, "Exception occurred while converting SOAP to REST", e); return new GenerateAPIResponse(null, null, "Exception occurred while converting SOAP to REST: " + e.getMessage()); } catch (MalformedURLException e) { LOGGER.log(Level.SEVERE, "Invalid WSDL URL", e); return new GenerateAPIResponse(null, null, "Invalid WSDL URL: " + e.getMessage()); } catch (Exception e) { LOGGER.log(Level.SEVERE, "Exception occurred while creating API from WSDL", e); return new GenerateAPIResponse(null, null, "Exception occurred while creating API from WSDL: " + e.getMessage()); } } - return null; + return new GenerateAPIResponse(null, null, "Unsupported API creation mode: " + mode);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/RestApiAdmin.java` around lines 123 - 147, Return an error payload instead of null when mode is unsupported or blank in RestApiAdmin’s generate path. Update the create API dispatch logic around createAPIFromSwagger/createAPIFromWSDL so the final fallback does not return null; instead, build a GenerateAPIResponse with a clear error message for invalid mode values and have SynapseLanguageService.generateAPI() propagate that response directly. This keeps the existing exception handling consistent and removes the need for callers to special-case null.
🧹 Nitpick comments (1)
org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/extensions/synapse/SynapseDiagnosticsParticipantTest.java (1)
519-526: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover script CDATA in raw-body exclusions.
The raw
<script>exclusion is only tested with plain text, while CDATA scanning is added separately. Add a CDATA script case so the exclusion is verified for both node forms.Proposed test coverage
public void testScriptBodyUnclosedExpressionNotFlagged() { // Raw-code (script) bodies are excluded — a ${ in JS is not a Synapse expression. String xml = "<sequence xmlns=\"" + SYNAPSE_NS + "\" name=\"test\">" + "<script language=\"js\">var s = \"${notReal\";</script>" + "</sequence>"; List<Diagnostic> diags = diagnosticsWithCode(diagnose(xml), "UnclosedExpression"); assertTrue(diags.isEmpty(), "Script bodies must not be scanned for unclosed expressions"); } + + `@Test` + public void testScriptCdataNotScannedForExpressions() { + String xml = "<sequence xmlns=\"" + SYNAPSE_NS + "\" name=\"test\">" + + "<script language=\"js\"><![CDATA[var s = \"${vars.notReal\";]]></script>" + + "</sequence>"; + List<Diagnostic> diags = diagnose(xml); + assertTrue(diagnosticsWithCode(diags, "UndefinedVariable").isEmpty(), + "Script CDATA should not be scanned for variable references"); + assertTrue(diagnosticsWithCode(diags, "UnclosedExpression").isEmpty(), + "Script CDATA should not be scanned for unclosed expressions"); + }Also applies to: 599-607, 611-634
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/extensions/synapse/SynapseDiagnosticsParticipantTest.java` around lines 519 - 526, The raw-body exclusion in Synapse diagnostics needs CDATA coverage in addition to the existing plain-text script case. Update the Synapse diagnostics tests around testScriptBodyNotScannedForVariables and the related CDATA scanning cases so a <script> node containing CDATA is also verified to skip variable scanning, while still asserting no UndefinedVariable diagnostics are produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 54-65: The release workflow is expanding step outputs directly
inside shell commands instead of passing them through environment variables
first. Update the release job around the release creation/upload logic to read
values from env vars (for example the version and file name from
steps.ls.outputs) and build the JSON payload with jq rather than inline string
interpolation. Keep the curl and jq flow in the same release creation/upload
block so the shell only uses sanitized env-backed values.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java`:
- Line 126: The synchronization in InboundConnectorHolder is incomplete because
fetchInboundConnectors() now updates shared mutable state asynchronously while
other public methods still access the same maps/JSON without the same
protection. Make all reads and writes to the holder state use one consistent
strategy across InboundConnectorHolder, especially around
getCustomInboundConnectors(), schema-related reads, and save/refresh paths, or
switch the mutating path to build and publish immutable snapshots before
exposing them.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/AbstractResourceFinder.java`:
- Around line 692-699: The filtering in AbstractResourceFinder.removeIf
currently compares the entire ArtifactResource protocol string, so multi-valued
attributes like “http https” are treated as one token and can be removed
incorrectly. Update the protocol matching logic in the resource filtering block
to split ArtifactResource.getProtocol() on whitespace, trim each individual
value, and compare each token against the requested protocols before deciding to
remove the resource.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/factory/APIFactory.java`:
- Around line 106-109: In APIFactory, the binds-to handling currently checks
only for null, so missing attributes still get set as empty values on the API
model. Update the logic around element.getAttribute(Constant.BINDS_TO) and
api.setBindsTo(...) to treat empty strings as absent, not present, so APIs
without binds-to do not carry an empty value into later serialization or
validation.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/factory/ResourceFactory.java`:
- Around line 98-101: The ResourceFactory logic is treating a missing binds-to
attribute as set because getAttribute(...) can return an empty string, so the
APIResource model ends up with an empty bindsTo value. Update the bindsTo
handling in ResourceFactory to check for non-empty content, not just non-null,
before calling apiResource.setBindsTo(...), so only real attribute values are
persisted.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/serializer/api/APISerializer.java`:
- Around line 62-64: Treat blank binds-to as absent by normalizing empty DOM
attribute values to null in the API parsing path, then keep APISerializer's
binds-to emission gated on a non-empty value. Update the APISerializer and the
corresponding parse/getter path around api.getBindsTo() / Constant.BINDS_TO so
omitted attributes do not round-trip as binds-to="".
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/serializer/api/ResourceSerializer.java`:
- Around line 54-56: Skip serializing blank binds-to values on resources:
ResourceSerializer currently writes any non-null Resource.getBindsTo() back out,
which turns empty strings into explicit binds-to="". Update the
ResourceSerializer logic to treat blank values as absent when adding the
Constant.BINDS_TO attribute, and align ResourceFactory so it preserves omission
for empty input instead of storing an empty string.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.java`:
- Around line 595-597: Add a null check in isMcpInboundEndpoint before accessing
element.getAttribute("class"), because Utils.getRootElementFromConfigXml(...)
can return null and DirectoryTreeBuilder may pass that through. Update the
helper to safely return false when the DOMElement is null, keeping the existing
Constant.MCP_INBOUND_LISTENER_CLASS comparison for non-null elements.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/extensions/synapse/SynapseDiagnosticsParticipant.java`:
- Around line 1378-1410: The unclosed-expression scan in hasUnclosedExpression
currently treats every "${" as an expression start, which causes false positives
when the opener appears inside quoted literals. Update hasUnclosedExpression
and/or hasClosingBrace in SynapseDiagnosticsParticipant to detect whether each
"${" is inside a quoted string before checking for a matching "}", and skip
those quoted occurrences entirely so only real expression openers are validated.
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/extensions/synapse/SynapseDiagnosticsParticipantTest.java`:
- Around line 1822-1823: The default-behavior test in
SynapseDiagnosticsParticipantTest is explicitly passing false to diagnoseAtPath,
which bypasses the actual default path and weakens regression coverage. Update
this test to use the two-argument diagnoseAtPath helper so it exercises the real
default behavior, and keep the assertion about the unresolved reference tied to
that default flow.
- Around line 1893-1908: Add an explicit assertion after Request A in
SynapseDiagnosticsParticipantTest to verify the participant has populated
duplicateArtifactNames before Request B runs, so the cleanup check is proven
against real stale state. Use the existing participant and diagnosticsWithCode
helpers to assert the duplicate state is present for “DupSeq” after
diagnoseAtPathWith, then keep the Request B assertion that doDiagnostics clears
it when no project index is available.
---
Outside diff comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/RestApiAdmin.java`:
- Around line 123-147: Return an error payload instead of null when mode is
unsupported or blank in RestApiAdmin’s generate path. Update the create API
dispatch logic around createAPIFromSwagger/createAPIFromWSDL so the final
fallback does not return null; instead, build a GenerateAPIResponse with a clear
error message for invalid mode values and have
SynapseLanguageService.generateAPI() propagate that response directly. This
keeps the existing exception handling consistent and removes the need for
callers to special-case null.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java`:
- Around line 126-168: The refresh logic in
InboundConnectorHolder#getCustomInboundConnectors only appends new connectors
and leaves stale entries behind in connectorIdMap, inboundConnectors, and the
temp JSON cache. Before iterating over the current zip set, clear the custom
inbound-connector state and rebuild it from scratch, then repopulate the JSON
array so getInboundConnectorSchema* and the init path all reflect the same
refreshed set. Keep the existing extraction/import flow, but make sure removed
connectors are no longer resolvable after a refresh.
---
Nitpick comments:
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/extensions/synapse/SynapseDiagnosticsParticipantTest.java`:
- Around line 519-526: The raw-body exclusion in Synapse diagnostics needs CDATA
coverage in addition to the existing plain-text script case. Update the Synapse
diagnostics tests around testScriptBodyNotScannedForVariables and the related
CDATA scanning cases so a <script> node containing CDATA is also verified to
skip variable scanning, while still asserting no UndefinedVariable diagnostics
are produced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c7c94a4-30d8-4597-b508-bb383b4a30c5
📒 Files selected for processing (51)
.github/workflows/release.ymlorg.eclipse.lemminx/pom.xmlorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/SynapseLanguageService.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLTextDocumentService.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLWorkspaceService.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/ISynapseLanguageService.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/CodeDiagnosticRequest.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/RestApiAdmin.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/pojo/GenerateAPIResponse.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/SchemaGenerate.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/generate/ConnectorGeneratorResponse.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/dependency/tree/OverviewModelGenerator.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/directoryTree/DirectoryMapResponse.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/directoryTree/DirectoryTreeBuilder.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/directoryTree/node/Node.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/directoryTree/node/Resource.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/expression/SemanticExpressionValidator.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/mediatorService/AIConnectorHandler.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/mediatorService/MediatorHandler.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/AbstractResourceFinder.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/NewProjectResourceFinder.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/pojo/ArtifactResource.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/pojo/RequestedResource.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/factory/APIFactory.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/factory/ResourceFactory.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/factory/mediators/ConnectorFactory.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/pojo/api/API.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/pojo/api/APIResource.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/pojo/connector/Connector.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/serializer/api/APISerializer.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/serializer/api/ResourceSerializer.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Constant.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/extensions/synapse/SynapseDiagnosticsParticipant.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/extensions/synapse/validator/SynapseExpressionValidator.javaorg.eclipse.lemminx/src/main/resources/org/eclipse/lemminx/inbound-endpoints/inbound_endpoints_460.jsonorg.eclipse.lemminx/src/main/resources/org/eclipse/lemminx/mediators/430/templates/connector.mustacheorg.eclipse.lemminx/src/main/resources/org/eclipse/lemminx/mediators/440/templates/connector.mustacheorg.eclipse.lemminx/src/main/resources/org/eclipse/lemminx/schemas/430/api.xsdorg.eclipse.lemminx/src/main/resources/org/eclipse/lemminx/schemas/430/misc/resource.xsdorg.eclipse.lemminx/src/main/resources/org/eclipse/lemminx/schemas/430/task.xsdorg.eclipse.lemminx/src/main/resources/org/eclipse/lemminx/schemas/440/api.xsdorg.eclipse.lemminx/src/main/resources/org/eclipse/lemminx/schemas/440/misc/resource.xsdorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/extensions/synapse/CodeDiagnosticFileNameTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/extensions/synapse/SynapseDiagnosticsParticipantTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/directorytree/builder/DirectoryTreeBuilderTest.javaorg.eclipse.lemminx/src/test/java/org/eclipse/lemminx/synapse/expression/ExpressionValidatorTest.javaorg.eclipse.lemminx/src/test/resources/synapse/directorytree.builder/generated-directory-tree.jsonorg.eclipse.lemminx/src/test/resources/synapse/directorytree.builder/generated-project-explorer.jsonpom.xml
Comment on lines
+54
to
65
| -d '{"tag_name":"v${{ steps.ls.outputs.version }}", "draft":false, "name": "Release v${{ steps.ls.outputs.version }}", "prerelease":true}' \ | ||
| https://api.github.com/repos/${{ github.repository }}/releases) | ||
| id=$(echo "$createResponse" | jq -r '.id // empty') | ||
| if [ -z "$id" ]; then | ||
| echo "Failed to parse release id from response:" | ||
| echo "$createResponse" | ||
| exit 1 | ||
| fi | ||
| curl --fail-with-body -X POST \ | ||
| -H "Authorization:token ${{ secrets.GIT_BOT_TOKEN }}" \ | ||
| -H "Content-Type:application/octet-stream" \ | ||
| --data-binary @${{ steps.ls.outputs.fileName }}.zip \ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pass workflow outputs through environment variables before shell use.
Use env vars and jq payload construction instead of expanding step outputs directly inside shell commands.
Proposed adjustment
- name: Create a release in repo
+ env:
+ GIT_BOT_TOKEN: ${{ secrets.GIT_BOT_TOKEN }}
+ LS_VERSION: ${{ steps.ls.outputs.version }}
+ LS_FILE_NAME: ${{ steps.ls.outputs.fileName }}
run: |
set -euo pipefail
+ releasePayload=$(jq -n \
+ --arg tag "v${LS_VERSION}" \
+ --arg name "Release v${LS_VERSION}" \
+ '{tag_name: $tag, draft: false, name: $name, prerelease: true}')
createResponse=$(curl --fail-with-body -X POST \
-H "Accept: application/vnd.github.v3+json" \
- -H "Authorization:token ${{ secrets.GIT_BOT_TOKEN }}" \
- -d '{"tag_name":"v${{ steps.ls.outputs.version }}", "draft":false, "name": "Release v${{ steps.ls.outputs.version }}", "prerelease":true}' \
+ -H "Authorization:token ${GIT_BOT_TOKEN}" \
+ -d "$releasePayload" \
https://api.github.com/repos/${{ github.repository }}/releases)
...
curl --fail-with-body -X POST \
- -H "Authorization:token ${{ secrets.GIT_BOT_TOKEN }}" \
+ -H "Authorization:token ${GIT_BOT_TOKEN}" \
-H "Content-Type:application/octet-stream" \
- --data-binary @${{ steps.ls.outputs.fileName }}.zip \
- "https://uploads.github.com/repos/${{ github.repository }}/releases/$id/assets?name=${{ steps.ls.outputs.fileName }}.zip"
+ --data-binary @"${LS_FILE_NAME}.zip" \
+ "https://uploads.github.com/repos/${{ github.repository }}/releases/$id/assets?name=${LS_FILE_NAME}.zip" - name: Create sync PR from stable/mi to main
if: github.ref == 'refs/heads/stable/mi'
env:
GH_TOKEN: ${{ secrets.GIT_BOT_TOKEN }}
+ LS_VERSION: ${{ steps.ls.outputs.version }}
...
- --title "Sync stable/mi to main after release v${{ steps.ls.outputs.version }}" \
- --body "Automated PR to sync \`stable/mi\` into \`main\` following the successful release of v${{ steps.ls.outputs.version }}."
+ --title "Sync stable/mi to main after release v${LS_VERSION}" \
+ --body "Automated PR to sync \`stable/mi\` into \`main\` following the successful release of v${LS_VERSION}."As per path instructions, provide concise, actionable feedback focused on correctness and best practices.
Also applies to: 88-89
🧰 Tools
🪛 zizmor (1.26.1)
[info] 54-54: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 54-54: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 65-65: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 65-65: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 54 - 65, The release workflow is
expanding step outputs directly inside shell commands instead of passing them
through environment variables first. Update the release job around the release
creation/upload logic to read values from env vars (for example the version and
file name from steps.ls.outputs) and build the JSON payload with jq rather than
inline string interpolation. Keep the curl and jq flow in the same release
creation/upload block so the shell only uses sanitized env-backed values.
Sources: Path instructions, Linters/SAST tools
| } | ||
|
|
||
| public void getCustomInboundConnectors() { | ||
| public synchronized String getCustomInboundConnectors() { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
This lock does not cover the holder's shared state.
fetchInboundConnectors() now runs asynchronously, but other public methods still read and write the same mutable maps/JSON without the same lock. Refresh can still race with schema reads or saves. Use one synchronization strategy across all holder state, or publish immutable snapshots from the mutating path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java`
at line 126, The synchronization in InboundConnectorHolder is incomplete because
fetchInboundConnectors() now updates shared mutable state asynchronously while
other public methods still access the same maps/JSON without the same
protection. Make all reads and writes to the holder state use one consistent
strategy across InboundConnectorHolder, especially around
getCustomInboundConnectors(), schema-related reads, and save/refresh paths, or
switch the mutating path to build and publish immutable snapshots before
exposing them.
Comment on lines
+692
to
+699
| response.getResources().removeIf(resource -> { | ||
| if (!requested.getType().equals(resource.getType())) { | ||
| return false; | ||
| } | ||
| String protocol = resource instanceof ArtifactResource | ||
| ? ((ArtifactResource) resource).getProtocol() : null; | ||
| return protocol == null || protocols.stream().noneMatch(p -> p.equalsIgnoreCase(protocol.trim())); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Split multi-valued protocol attributes before filtering.
Line 698 compares against the full protocol attribute string. Artifacts can carry multiple space-separated protocols, so a value like "http https" will be filtered out even when the request asks for http or https.
Suggested fix
response.getResources().removeIf(resource -> {
if (!requested.getType().equals(resource.getType())) {
return false;
}
String protocol = resource instanceof ArtifactResource
? ((ArtifactResource) resource).getProtocol() : null;
- return protocol == null || protocols.stream().noneMatch(p -> p.equalsIgnoreCase(protocol.trim()));
+ if (protocol == null) {
+ return true;
+ }
+ return Arrays.stream(protocol.trim().split("\\s+"))
+ .filter(value -> !value.isEmpty())
+ .noneMatch(resourceProtocol ->
+ protocols.stream().anyMatch(requestedProtocol ->
+ requestedProtocol.equalsIgnoreCase(resourceProtocol)));
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| response.getResources().removeIf(resource -> { | |
| if (!requested.getType().equals(resource.getType())) { | |
| return false; | |
| } | |
| String protocol = resource instanceof ArtifactResource | |
| ? ((ArtifactResource) resource).getProtocol() : null; | |
| return protocol == null || protocols.stream().noneMatch(p -> p.equalsIgnoreCase(protocol.trim())); | |
| }); | |
| response.getResources().removeIf(resource -> { | |
| if (!requested.getType().equals(resource.getType())) { | |
| return false; | |
| } | |
| String protocol = resource instanceof ArtifactResource | |
| ? ((ArtifactResource) resource).getProtocol() : null; | |
| if (protocol == null) { | |
| return true; | |
| } | |
| return Arrays.stream(protocol.trim().split("\\s+")) | |
| .filter(value -> !value.isEmpty()) | |
| .noneMatch(resourceProtocol -> | |
| protocols.stream().anyMatch(requestedProtocol -> | |
| requestedProtocol.equalsIgnoreCase(resourceProtocol))); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/resourceFinder/AbstractResourceFinder.java`
around lines 692 - 699, The filtering in AbstractResourceFinder.removeIf
currently compares the entire ArtifactResource protocol string, so multi-valued
attributes like “http https” are treated as one token and can be removed
incorrectly. Update the protocol matching logic in the resource filtering block
to split ArtifactResource.getProtocol() on whitespace, trim each individual
value, and compare each token against the requested protocols before deciding to
remove the resource.
Comment on lines
+106
to
+109
| String bindsTo = element.getAttribute(Constant.BINDS_TO); | ||
| if (Objects.nonNull(bindsTo)) { | ||
| api.setBindsTo(bindsTo); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve missing binds-to as absent.
element.getAttribute(...) yields "" when the attribute is missing, so this code sets bindsTo for APIs that do not declare binds-to. That loses the distinction between “unset” and “empty” and can leak empty values into later serialization or validation. Guard on emptiness instead.
Suggested fix
String bindsTo = element.getAttribute(Constant.BINDS_TO);
- if (Objects.nonNull(bindsTo)) {
+ if (!bindsTo.isEmpty()) {
api.setBindsTo(bindsTo);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| String bindsTo = element.getAttribute(Constant.BINDS_TO); | |
| if (Objects.nonNull(bindsTo)) { | |
| api.setBindsTo(bindsTo); | |
| } | |
| String bindsTo = element.getAttribute(Constant.BINDS_TO); | |
| if (!bindsTo.isEmpty()) { | |
| api.setBindsTo(bindsTo); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/factory/APIFactory.java`
around lines 106 - 109, In APIFactory, the binds-to handling currently checks
only for null, so missing attributes still get set as empty values on the API
model. Update the logic around element.getAttribute(Constant.BINDS_TO) and
api.setBindsTo(...) to treat empty strings as absent, not present, so APIs
without binds-to do not carry an empty value into later serialization or
validation.
Source: Path instructions
Comment on lines
+98
to
+101
| String bindsTo = element.getAttribute(Constant.BINDS_TO); | ||
| if (Objects.nonNull(bindsTo)) { | ||
| apiResource.setBindsTo(bindsTo); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Avoid setting bindsTo for resources when the attribute is absent.
Here too, getAttribute(...) returns "" for a missing binds-to, so the model records an empty value instead of “not set”. Use an emptiness check before calling setBindsTo(...).
Suggested fix
String bindsTo = element.getAttribute(Constant.BINDS_TO);
- if (Objects.nonNull(bindsTo)) {
+ if (!bindsTo.isEmpty()) {
apiResource.setBindsTo(bindsTo);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| String bindsTo = element.getAttribute(Constant.BINDS_TO); | |
| if (Objects.nonNull(bindsTo)) { | |
| apiResource.setBindsTo(bindsTo); | |
| } | |
| String bindsTo = element.getAttribute(Constant.BINDS_TO); | |
| if (!bindsTo.isEmpty()) { | |
| apiResource.setBindsTo(bindsTo); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/factory/ResourceFactory.java`
around lines 98 - 101, The ResourceFactory logic is treating a missing binds-to
attribute as set because getAttribute(...) can return an empty string, so the
APIResource model ends up with an empty bindsTo value. Update the bindsTo
handling in ResourceFactory to check for non-empty content, not just non-null,
before calling apiResource.setBindsTo(...), so only real attribute values are
persisted.
Source: Path instructions
Comment on lines
+54
to
+56
| if (resource.getBindsTo() != null) { | ||
| resourceElt.addAttribute(Constant.BINDS_TO, resource.getBindsTo(), null); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resource binds-to parse/serialize paths =="
rg -n -C2 'ResourceFactory|serializeResource|getAttribute\(Constant\.BINDS_TO\)|addAttribute\(Constant\.BINDS_TO' \
org.eclipse.lemminx/src/main/javaRepository: wso2/mi-language-server
Length of output: 14060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '90,110p' org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/factory/ResourceFactory.java
printf '\n---\n'
sed -n '1,120p' org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/model/APIResource.java
printf '\n---\n'
sed -n '1,90p' org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/serializer/api/ResourceSerializer.javaRepository: wso2/mi-language-server
Length of output: 1094
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate APIResource model =="
fd -a 'APIResource.java' org.eclipse.lemminx/src/main/java
echo
echo "== resource factory + serializer context =="
sed -n '90,110p' org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/factory/ResourceFactory.java
printf '\n---\n'
sed -n '1,120p' org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/serializer/api/ResourceSerializer.javaRepository: wso2/mi-language-server
Length of output: 4888
Skip serializing blank binds-to on resources. ResourceFactory stores binds-to="" as a value, and ResourceSerializer writes any non-null value back out, so empty attributes become explicit binds-to="" instead of staying omitted. Use a non-blank check on both sides.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/syntaxTree/serializer/api/ResourceSerializer.java`
around lines 54 - 56, Skip serializing blank binds-to values on resources:
ResourceSerializer currently writes any non-null Resource.getBindsTo() back out,
which turns empty strings into explicit binds-to="". Update the
ResourceSerializer logic to treat blank values as absent when adding the
Constant.BINDS_TO attribute, and align ResourceFactory so it preserves omission
for empty input instead of storing an empty string.
Comment on lines
+595
to
+597
| public static boolean isMcpInboundEndpoint(DOMElement element) { | ||
| return Constant.MCP_INBOUND_LISTENER_CLASS.equals(element.getAttribute("class")); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep --pattern $'Utils.isMcpInboundEndpoint($_)' --lang java
rg -nP -C4 'getRootElementFromConfigXml' org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/directoryTree/DirectoryTreeBuilder.javaRepository: wso2/mi-language-server
Length of output: 1616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Utils.isMcpInboundEndpoint =="
sed -n '585,605p' org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.java
echo
echo "== DirectoryTreeBuilder around inbound endpoint call site =="
sed -n '736,762p' org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/directoryTree/DirectoryTreeBuilder.java
echo
echo "== DirectoryTreeBuilder rootElement assignment context =="
sed -n '616,630p' org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/directoryTree/DirectoryTreeBuilder.javaRepository: wso2/mi-language-server
Length of output: 3079
Add a null guard to isMcpInboundEndpoint.
DirectoryTreeBuilder passes Utils.getRootElementFromConfigXml(...) straight into this helper, and that method can return null, so the inbound-endpoint path can throw.
🛡️ Proposed guard
public static boolean isMcpInboundEndpoint(DOMElement element) {
- return Constant.MCP_INBOUND_LISTENER_CLASS.equals(element.getAttribute("class"));
+ return element != null
+ && Constant.MCP_INBOUND_LISTENER_CLASS.equals(element.getAttribute("class"));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static boolean isMcpInboundEndpoint(DOMElement element) { | |
| return Constant.MCP_INBOUND_LISTENER_CLASS.equals(element.getAttribute("class")); | |
| } | |
| public static boolean isMcpInboundEndpoint(DOMElement element) { | |
| return element != null | |
| && Constant.MCP_INBOUND_LISTENER_CLASS.equals(element.getAttribute("class")); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.java`
around lines 595 - 597, Add a null check in isMcpInboundEndpoint before
accessing element.getAttribute("class"), because
Utils.getRootElementFromConfigXml(...) can return null and DirectoryTreeBuilder
may pass that through. Update the helper to safely return false when the
DOMElement is null, keeping the existing Constant.MCP_INBOUND_LISTENER_CLASS
comparison for non-null elements.
Comment on lines
+1378
to
+1410
| private boolean hasUnclosedExpression(String value) { | ||
| int open = value.indexOf("${"); | ||
| while (open >= 0) { | ||
| if (!hasClosingBrace(value, open + 2)) { | ||
| return true; | ||
| } | ||
| open = value.indexOf("${", open + 2); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true if there is a '}' at or after {@code from} that lies outside any string literal. | ||
| */ | ||
| private boolean hasClosingBrace(String value, int from) { | ||
| boolean inString = false; | ||
| char quote = 0; | ||
| for (int i = from; i < value.length(); i++) { | ||
| char c = value.charAt(i); | ||
| if (inString) { | ||
| if (c == '\\') { | ||
| i++; // skip the escaped character | ||
| } else if (c == quote) { | ||
| inString = false; | ||
| } | ||
| } else if (c == '"' || c == '\'') { | ||
| inString = true; | ||
| quote = c; | ||
| } else if (c == '}') { | ||
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ignore ${ inside quoted literals when scanning for unclosed expressions.
hasUnclosedExpression() currently treats every ${ as a candidate expression start. In text/CDATA payloads, that produces false UnclosedExpression warnings for quoted literal content because hasClosingBrace() starts scanning mid-string and cannot tell the opener was already inside quotes. Please skip ${ occurrences that are inside quoted literals before checking for a closing brace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/extensions/synapse/SynapseDiagnosticsParticipant.java`
around lines 1378 - 1410, The unclosed-expression scan in hasUnclosedExpression
currently treats every "${" as an expression start, which causes false positives
when the opener appears inside quoted literals. Update hasUnclosedExpression
and/or hasClosingBrace in SynapseDiagnosticsParticipant to detect whether each
"${" is inside a quoted string before checking for a matching "}", and skip
those quoted occurrences entirely so only real expression openers are validated.
Comment on lines
+1822
to
+1823
| // Default (flag false) — cross-file validation runs and flags the unresolved reference. | ||
| List<Diagnostic> diags = diagnoseAtPath(xml, apiXml, false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the default helper for the default-behavior test.
This test explicitly sets skipCrossFile=false, so it would not catch a regression in the actual default state. Call the two-argument helper instead.
Proposed fix
- List<Diagnostic> diags = diagnoseAtPath(xml, apiXml, false);
+ List<Diagnostic> diags = diagnoseAtPath(xml, apiXml);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Default (flag false) — cross-file validation runs and flags the unresolved reference. | |
| List<Diagnostic> diags = diagnoseAtPath(xml, apiXml, false); | |
| // Default (flag false) — cross-file validation runs and flags the unresolved reference. | |
| List<Diagnostic> diags = diagnoseAtPath(xml, apiXml); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/extensions/synapse/SynapseDiagnosticsParticipantTest.java`
around lines 1822 - 1823, The default-behavior test in
SynapseDiagnosticsParticipantTest is explicitly passing false to diagnoseAtPath,
which bypasses the actual default path and weakens regression coverage. Update
this test to use the two-argument diagnoseAtPath helper so it exercises the real
default behavior, and keep the assertion about the unresolved reference tied to
that default flow.
Comment on lines
+1893
to
+1908
| // Request A: validate a doc inside the project so the duplicate index is built into the | ||
| // participant's instance state (duplicateArtifactNames = { "DupSeq" }). | ||
| Path apiXml = project.resolve("src/main/wso2mi/artifacts/apis/cvh.xml"); | ||
| diagnoseAtPathWith(participant, "<api xmlns=\"" + SYNAPSE_NS + "\" name=\"cvh\" context=\"/cvh\">" | ||
| + "<resource methods=\"GET\" uri-template=\"/\"><inSequence><respond/></inSequence></resource></api>", | ||
| apiXml); | ||
|
|
||
| // Request B (same participant): a doc named "DupSeq" whose project path is not derivable, so | ||
| // the cross-file index is unavailable. The stale duplicate state must be cleared, not reused. | ||
| TextDocument textB = new TextDocument( | ||
| "<sequence xmlns=\"" + SYNAPSE_NS + "\" name=\"DupSeq\"><log/></sequence>", "test.xml"); | ||
| DOMDocument docB = DOMParser.getInstance().parse(textB, null); | ||
| List<Diagnostic> diagsB = new ArrayList<>(); | ||
| participant.doDiagnostics(docB, diagsB, null, () -> {}); | ||
| assertTrue(diagnosticsWithCode(diagsB, "DuplicateArtifactName").isEmpty(), | ||
| "Stale cross-file duplicate state must not leak to a request with no project index"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the stale-state precondition before checking cleanup.
Request A should prove the participant has duplicate state before Request B verifies it is cleared; otherwise this can pass without exercising the intended path.
Proposed fix
- Path apiXml = project.resolve("src/main/wso2mi/artifacts/apis/cvh.xml");
- diagnoseAtPathWith(participant, "<api xmlns=\"" + SYNAPSE_NS + "\" name=\"cvh\" context=\"/cvh\">"
- + "<resource methods=\"GET\" uri-template=\"/\"><inSequence><respond/></inSequence></resource></api>",
- apiXml);
+ List<Diagnostic> diagsA = diagnoseAtPathWith(participant,
+ "<sequence xmlns=\"" + SYNAPSE_NS + "\" name=\"DupSeq\"><log/></sequence>",
+ seqA);
+ assertFalse(diagnosticsWithCode(diagsA, "DuplicateArtifactName").isEmpty(),
+ "Request A must populate duplicate state before Request B verifies cleanup");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Request A: validate a doc inside the project so the duplicate index is built into the | |
| // participant's instance state (duplicateArtifactNames = { "DupSeq" }). | |
| Path apiXml = project.resolve("src/main/wso2mi/artifacts/apis/cvh.xml"); | |
| diagnoseAtPathWith(participant, "<api xmlns=\"" + SYNAPSE_NS + "\" name=\"cvh\" context=\"/cvh\">" | |
| + "<resource methods=\"GET\" uri-template=\"/\"><inSequence><respond/></inSequence></resource></api>", | |
| apiXml); | |
| // Request B (same participant): a doc named "DupSeq" whose project path is not derivable, so | |
| // the cross-file index is unavailable. The stale duplicate state must be cleared, not reused. | |
| TextDocument textB = new TextDocument( | |
| "<sequence xmlns=\"" + SYNAPSE_NS + "\" name=\"DupSeq\"><log/></sequence>", "test.xml"); | |
| DOMDocument docB = DOMParser.getInstance().parse(textB, null); | |
| List<Diagnostic> diagsB = new ArrayList<>(); | |
| participant.doDiagnostics(docB, diagsB, null, () -> {}); | |
| assertTrue(diagnosticsWithCode(diagsB, "DuplicateArtifactName").isEmpty(), | |
| "Stale cross-file duplicate state must not leak to a request with no project index"); | |
| // Request A: validate a doc inside the project so the duplicate index is built into the | |
| // participant's instance state (duplicateArtifactNames = { "DupSeq" }). | |
| List<Diagnostic> diagsA = diagnoseAtPathWith(participant, | |
| "<sequence xmlns=\"" + SYNAPSE_NS + "\" name=\"DupSeq\"><log/></sequence>", | |
| seqA); | |
| assertFalse(diagnosticsWithCode(diagsA, "DuplicateArtifactName").isEmpty(), | |
| "Request A must populate duplicate state before Request B verifies cleanup"); | |
| // Request B (same participant): a doc named "DupSeq" whose project path is not derivable, so | |
| // the cross-file index is unavailable. The stale duplicate state must be cleared, not reused. | |
| TextDocument textB = new TextDocument( | |
| "<sequence xmlns=\"" + SYNAPSE_NS + "\" name=\"DupSeq\"><log/></sequence>", "test.xml"); | |
| DOMDocument docB = DOMParser.getInstance().parse(textB, null); | |
| List<Diagnostic> diagsB = new ArrayList<>(); | |
| participant.doDiagnostics(docB, diagsB, null, () -> {}); | |
| assertTrue(diagnosticsWithCode(diagsB, "DuplicateArtifactName").isEmpty(), | |
| "Stale cross-file duplicate state must not leak to a request with no project index"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/extensions/synapse/SynapseDiagnosticsParticipantTest.java`
around lines 1893 - 1908, Add an explicit assertion after Request A in
SynapseDiagnosticsParticipantTest to verify the participant has populated
duplicateArtifactNames before Request B runs, so the cleanup check is proven
against real stale state. Use the existing participant and diagnosticsWithCode
helpers to assert the duplicate state is present for “DupSeq” after
diagnoseAtPathWith, then keep the Request B assertion that doDiagnostics clears
it when no project index is available.
Change custom inbound-endpoints directory
Fix solace description retrieving issue
Update LS version
Handle inbound-endpoints with same ID
Update connector gen tool version
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java (1)
157-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParse the schema once and guard against null.
Utils.getJsonObject(schema)is called twice on the same string, redundantly re-parsing the JSON. The first call at line 157 also accesses.get(Constant.NAME).getAsString()without checking whether the returned object is null.♻️ Proposed refactor
- if (saveInboundConnector(Utils.getJsonObject(schema).get(Constant.NAME).getAsString(), schema)) { - JsonObject connectorSchema = Utils.getJsonObject(schema); + JsonObject connectorSchema = Utils.getJsonObject(schema); + if (connectorSchema != null && connectorSchema.has(Constant.NAME) && + saveInboundConnector(connectorSchema.get(Constant.NAME).getAsString(), schema)) { JsonArray connectorArray = this.inboundConnectorListJson.getAsJsonArray(Constant.INBOUND_CONNECTOR_DATA);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java` around lines 157 - 158, In the inbound connector handling logic, parse the schema once into a JsonObject before the saveInboundConnector check, validate that the parsed object and its Constant.NAME element are non-null, then derive the connector name and reuse the same JsonObject for subsequent processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java`:
- Around line 130-134: Update the resource-loading logic in the
InboundConnectorHolder method containing JsonLoader.getResourceAsStream:
validate that the returned InputStream is non-null and handle the
missing-resource case with the method’s appropriate exception or error path.
Wrap the InputStream and BufferedReader in try-with-resources, then parse the
JSON within that scope so both resources are always closed.
---
Nitpick comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java`:
- Around line 157-158: In the inbound connector handling logic, parse the schema
once into a JsonObject before the saveInboundConnector check, validate that the
parsed object and its Constant.NAME element are non-null, then derive the
connector name and reuse the same JsonObject for subsequent processing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: adc80e3a-846a-497f-9322-0d425c1cbc84
📒 Files selected for processing (6)
org.eclipse.lemminx/pom.xmlorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLWorkspaceService.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/directoryTree/DirectoryTreeBuilder.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/UISchemaMapper.javapom.xml
✅ Files skipped from review due to trivial changes (1)
- pom.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/XMLWorkspaceService.java
- org.eclipse.lemminx/pom.xml
- org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/directoryTree/DirectoryTreeBuilder.java
Comment on lines
130
to
134
| InputStream inputStream = JsonLoader.class | ||
| .getResourceAsStream("/org/eclipse/lemminx/inbound-endpoints/inbound_endpoints_" | ||
| + this.projectRuntimeVersion.replace(".", StringUtils.EMPTY) + Constant.JSON_FILE_EXT); | ||
| BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream)); | ||
| this.inboundConnectorListJson = JsonParser.parseReader(reader).getAsJsonObject(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close the stream and guard against a missing resource.
The InputStream and BufferedReader are never closed, leaking a file descriptor on every call. Additionally, getResourceAsStream returns null when the resource is absent, which causes an InputStreamReader constructor NPE that propagates uncaught through the async caller.
🔧 Proposed fix
public synchronized String getCustomInboundConnectors() {
boolean isInboundConnectorAdded = false;
- InputStream inputStream = JsonLoader.class
- .getResourceAsStream("/org/eclipse/lemminx/inbound-endpoints/inbound_endpoints_"
- + this.projectRuntimeVersion.replace(".", StringUtils.EMPTY) + Constant.JSON_FILE_EXT);
- BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream));
- this.inboundConnectorListJson = JsonParser.parseReader(reader).getAsJsonObject();
+ String resourcePath = "/org/eclipse/lemminx/inbound-endpoints/inbound_endpoints_"
+ + this.projectRuntimeVersion.replace(".", StringUtils.EMPTY) + Constant.JSON_FILE_EXT;
+ try (InputStream inputStream = JsonLoader.class.getResourceAsStream(resourcePath)) {
+ if (inputStream == null) {
+ LOGGER.log(Level.SEVERE, "Inbound connector list resource not found: " + resourcePath);
+ return "Failed to import the inbound-connector";
+ }
+ try (BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream))) {
+ this.inboundConnectorListJson = JsonParser.parseReader(reader).getAsJsonObject();
+ }
+ } catch (IOException e) {
+ LOGGER.log(Level.SEVERE, "Failed to read inbound connector list resource.", e);
+ return "Failed to import the inbound-connector";
+ }
Path resourcesPath = Path.of(this.projectPath, Constant.SRC, Constant.MAIN, Constant.WSO2MI,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| InputStream inputStream = JsonLoader.class | |
| .getResourceAsStream("/org/eclipse/lemminx/inbound-endpoints/inbound_endpoints_" | |
| + this.projectRuntimeVersion.replace(".", StringUtils.EMPTY) + Constant.JSON_FILE_EXT); | |
| BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream)); | |
| this.inboundConnectorListJson = JsonParser.parseReader(reader).getAsJsonObject(); | |
| boolean isInboundConnectorAdded = false; | |
| String resourcePath = "/org/eclipse/lemminx/inbound-endpoints/inbound_endpoints_" | |
| this.projectRuntimeVersion.replace(".", StringUtils.EMPTY) + Constant.JSON_FILE_EXT; | |
| try (InputStream inputStream = JsonLoader.class.getResourceAsStream(resourcePath)) { | |
| if (inputStream == null) { | |
| LOGGER.log(Level.SEVERE, "Inbound connector list resource not found: " + resourcePath); | |
| return "Failed to import the inbound-connector"; | |
| } | |
| try (BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream))) { | |
| this.inboundConnectorListJson = JsonParser.parseReader(reader).getAsJsonObject(); | |
| } | |
| } catch (IOException e) { | |
| LOGGER.log(Level.SEVERE, "Failed to read inbound connector list resource.", e); | |
| return "Failed to import the inbound-connector"; | |
| } | |
| Path resourcesPath = Path.of(this.projectPath, Constant.SRC, Constant.MAIN, Constant.WSO2MI, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java`
around lines 130 - 134, Update the resource-loading logic in the
InboundConnectorHolder method containing JsonLoader.getResourceAsStream:
validate that the returned InputStream is non-null and handle the
missing-resource case with the method’s appropriate exception or error path.
Wrap the InputStream and BufferedReader in try-with-resources, then parse the
JSON within that scope so both resources are always closed.
Add ASB inbound-endpoint support
Add inbound connector input variable suggestion support
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/entity/ConnectorVariableSchemaUtils.java (1)
71-107: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCircular
$refsilently drops the property instead of truncating it.When a definition reference has already been visited on the current path, the code
continues without adding anyPropertyfor that key (Lines 79-106). The field disappears from the output tree entirely rather than being represented (e.g., with an emptypropertieslist) to break the cycle. For self-referencing schema definitions, this can produce an incomplete variable tree for tryout/UI consumers.♻️ Suggested fix: still add the property node, just stop recursing further
if (!processedRefs.contains(definitionKey)) { JsonObject definitionObj = definitions.getAsJsonObject(definitionKey); if (definitionObj != null) { // Create property with the key from the property name Property property = new Property(key, StringUtils.EMPTY); // Get description from the definition if available if (definitionObj.has(Constant.DESCRIPTION)) { property.setDescription(definitionObj.get(Constant.DESCRIPTION).getAsString()); } // Extract nested properties from the definition if (definitionObj.has(Constant.PROPERTIES)) { Set<String> nestedRefs = new HashSet<>(processedRefs); nestedRefs.add(definitionKey); List<Property> nestedProps = extractProperties( definitionObj.getAsJsonObject(Constant.PROPERTIES), definitions, nestedRefs ); property.setProperties(nestedProps); } propertiesList.add(property); } + } else { + // Still represent the field, but stop recursion to avoid the cycle. + propertiesList.add(new Property(key, StringUtils.EMPTY)); } continue;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/entity/ConnectorVariableSchemaUtils.java` around lines 71 - 107, Update the circular-reference handling in extractProperties so an already visited definition still creates and adds a Property for the current key, but skips nested recursion and leaves its properties empty. Preserve the existing path-scoped processedRefs behavior and continue handling non-circular references as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java`:
- Around line 173-174: Prevent null dereferences while reading connector names:
in InboundConnectorHolder.java lines 173-174, guard connectorSchema and
Constant.NAME before calling getAsString; in AbstractConnectorLoader.java lines
164-165, similarly guard uiSchemaJson and the name property; in
NewProjectConnectorLoader.java line 122, store Utils.getJsonObject(schema) in a
local variable and null-check it before accessing properties.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/mediator/schema/generate/ServerLessTryoutHandler.java`:
- Around line 80-92: Update getSTNode to report an empty filePath using one of
the checked exception types already handled by handle, rather than throwing
IllegalArgumentException. Preserve the existing error message and ensure handle
returns MediatorTryoutInfo for empty paths through its current catch block.
- Around line 52-72: Update the edited-request flow in ServerLessTryoutHandler
so the InboundEndpoint branch obtains its STNode from editFilePath after copying
and applying edits, rather than parsing request.getFile(). Ensure subsequent
sequence resolution and tryout processing use the edited temporary endpoint
content.
---
Nitpick comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/entity/ConnectorVariableSchemaUtils.java`:
- Around line 71-107: Update the circular-reference handling in
extractProperties so an already visited definition still creates and adds a
Property for the current key, but skips nested recursion and leaves its
properties empty. Preserve the existing path-scoped processedRefs behavior and
continue handling non-circular references as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 104109f0-812c-43ee-94b3-335990c5283e
📒 Files selected for processing (8)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/AbstractConnectorLoader.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/NewProjectConnectorLoader.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/entity/ConnectorAction.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/entity/ConnectorVariableSchemaUtils.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/mediator/schema/generate/ServerLessTryoutHandler.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/mediator/schema/generate/visitor/InboundEndpointVisitor.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Constant.java
🚧 Files skipped from review as they are similar to previous changes (1)
- org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Constant.java
Comment on lines
173
to
+174
| JsonObject connectorSchema = Utils.getJsonObject(schema); | ||
| newConnector.addProperty(Constant.NAME, connectorSchema.get(Constant.TITLE) != null ? | ||
| connectorSchema.get(Constant.TITLE).getAsString() : StringUtils.EMPTY); | ||
| newConnector.addProperty(Constant.ID, connectorSchema.get(Constant.ID) != null ? | ||
| connectorSchema.get(Constant.ID).getAsString() : StringUtils.EMPTY); | ||
| newConnector.addProperty(Constant.DESCRIPTION, connectorSchema.get(Constant.DESCRIPTION) != null ? | ||
| connectorSchema.get(Constant.DESCRIPTION).getAsString() : StringUtils.EMPTY); | ||
| newConnector.addProperty(Constant.TYPE, Constant.INBOUND_DASH_ENDPOINT); | ||
| JsonArray connectorArray = this.inboundConnectorListJson.getAsJsonArray(Constant.INBOUND_CONNECTOR_DATA); | ||
| connectorArray.add(newConnector); | ||
| if (saveInboundConnector(connectorSchema.get(Constant.NAME).getAsString(), schema)) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent NullPointerException during schema parsing.
Utils.getJsonObject(schema) can return null if the schema is malformed or invalid. Directly chaining .get(Constant.NAME).getAsString() without null or existence checks risks an uncaught NullPointerException, which can abruptly terminate the connector loading or cleanup processes. As per path instructions, ensure correctness and validate safety by guarding these accesses.
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java#L173-L174: Guard theconnectorSchemaaccess with a null check (e.g.,connectorSchema != null && connectorSchema.has(Constant.NAME)) before reading the property.org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/AbstractConnectorLoader.java#L164-L165: Guard theuiSchemaJsonaccess with a similar null and existence check before retrieving the name.org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/NewProjectConnectorLoader.java#L122-L122: EvaluateUtils.getJsonObject(schema)into a local variable and guard it with a null check before attempting to read its properties.
📍 Affects 3 files
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java#L173-L174(this comment)org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/AbstractConnectorLoader.java#L164-L165org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/connectors/NewProjectConnectorLoader.java#L122-L122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/inbound/conector/InboundConnectorHolder.java`
around lines 173 - 174, Prevent null dereferences while reading connector names:
in InboundConnectorHolder.java lines 173-174, guard connectorSchema and
Constant.NAME before calling getAsString; in AbstractConnectorLoader.java lines
164-165, similarly guard uiSchemaJson and the name property; in
NewProjectConnectorLoader.java line 122, store Utils.getJsonObject(schema) in a
local variable and null-check it before accessing properties.
Source: Path instructions
Comment on lines
+52
to
72
| String visitFilePath = request.getFile(); | ||
| if (request.getEdits() != null) { | ||
| STNode node = getSTNode(request.getFile()); | ||
| String documentUri = request.getFile(); | ||
| String editFilePath = TEMP_FOLDER.resolve(TEMP_FILE_NAME).toString(); | ||
| if (node instanceof InboundEndpoint) { | ||
| String sequence = ((InboundEndpoint) node).getSequence(); | ||
| if (StringUtils.isNotEmpty(sequence)) { | ||
| String seqPath = ConfigFinder.findEsbComponentPath(sequence, Constant.SEQUENCES, projectUri); | ||
| if (StringUtils.isNotEmpty(seqPath)) { | ||
| documentUri = seqPath; | ||
| } | ||
| } | ||
| } else { | ||
| visitFilePath = editFilePath; | ||
| } | ||
| Utils.copyFile(documentUri, TEMP_FOLDER.toString(), TEMP_FILE_NAME); | ||
| filePath = TEMP_FOLDER.resolve(TEMP_FILE_NAME).toString(); | ||
| TryOutUtils.doEdits(request.getEdits(), Path.of(filePath)); | ||
| request = new MediatorTryoutRequest(filePath, request.getLine(), request.getColumn() + 1, | ||
| TryOutUtils.doEdits(request.getEdits(), Path.of(editFilePath)); | ||
| request = new MediatorTryoutRequest(editFilePath, request.getLine(), request.getColumn() + 1, | ||
| request.getInputPayload(), null); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect visitSequenceByKey to see whether it uses request.getFile() (the edited temp copy)
# or re-resolves `sequence` via projectPath, bypassing the edited content.
ast-grep run --pattern 'visitSequenceByKey($$$) { $$$ }' --lang java org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.java
rg -n -A20 'static.*visitSequenceByKey' org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.javaRepository: wso2/mi-language-server
Length of output: 484
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate relevant symbols =="
rg -n "visitSequenceByKey|InboundEndpointVisitor|getDOMDocument|doEdits|TEMP_FILE_NAME|visitFilePath" org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse -S
echo
echo "== Read Utils.visitSequenceByKey =="
file="org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/utils/Utils.java"
line=$(rg -n "visitSequenceByKey" "$file" | head -n1 | cut -d: -f1)
start=$((line-20)); [ "$start" -lt 1 ] && start=1
end=$((line+80))
sed -n "${start},${end}p" "$file"
echo
echo "== Read ServerLessTryoutHandler around the reported lines =="
file="org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/mediator/schema/generate/ServerLessTryoutHandler.java"
sed -n '40,95p' "$file"
echo
echo "== Read InboundEndpointVisitor =="
file="org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/visitor/InboundEndpointVisitor.java"
sed -n '1,220p' "$file"Repository: wso2/mi-language-server
Length of output: 20190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Read visitor Utils.visitSequenceByKey =="
file="org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/mediator/schema/generate/visitor/Utils.java"
line=$(rg -n "public static void visitSequenceByKey" "$file" | head -n1 | cut -d: -f1)
start=$((line-25)); [ "$start" -lt 1 ] && start=1
end=$((line+90))
sed -n "${start},${end}p" "$file"
echo
echo "== Read schema-generate InboundEndpointVisitor =="
file="org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/mediator/schema/generate/visitor/InboundEndpointVisitor.java"
sed -n '1,160p' "$file"
echo
echo "== Read ServerLessTryoutHandler around the branch =="
file="org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/mediator/schema/generate/ServerLessTryoutHandler.java"
sed -n '48,78p' "$file"Repository: wso2/mi-language-server
Length of output: 8262
InboundEndpoint tryouts ignore endpoint edits ServerLessTryoutHandler.java:52-72 — the inbound-endpoint branch still parses request.getFile(), and Utils.visitSequenceByKey(...) re-resolves the sequence from projectPath instead of reading the edited temp copy. Build the node from editFilePath here as well, or edits to the endpoint file will be dropped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/mediator/schema/generate/ServerLessTryoutHandler.java`
around lines 52 - 72, Update the edited-request flow in ServerLessTryoutHandler
so the InboundEndpoint branch obtains its STNode from editFilePath after copying
and applying edits, rather than parsing request.getFile(). Ensure subsequent
sequence resolution and tryout processing use the edited temporary endpoint
content.
Comment on lines
+80
to
+92
| } catch (IOException | InvalidConfigurationException e) { | ||
| return new MediatorTryoutInfo(e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| private STNode getSTNode(String filePath) throws IOException, InvalidConfigurationException { | ||
|
|
||
| if (StringUtils.isEmpty(filePath)) { | ||
| throw new IllegalArgumentException("FilePath is null"); | ||
| } | ||
| DOMDocument domDocument = Utils.getDOMDocument(new File(filePath)); | ||
| return SyntaxTreeGenerator.buildTree(domDocument.getDocumentElement()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
getSTNode's IllegalArgumentException escapes the method's own error contract.
getSTNode declares throws IOException, InvalidConfigurationException but throws an unchecked IllegalArgumentException for an empty path (Line 88). The catch (IOException | InvalidConfigurationException e) in handle(...) (Line 80) does not cover this, so an empty request.getFile() propagates an unhandled runtime exception instead of returning a MediatorTryoutInfo with an error message like every other failure path in this method.
🐛 Suggested fix: throw a checked type already caught by `handle`
private STNode getSTNode(String filePath) throws IOException, InvalidConfigurationException {
if (StringUtils.isEmpty(filePath)) {
- throw new IllegalArgumentException("FilePath is null");
+ throw new InvalidConfigurationException("FilePath is null");
}
DOMDocument domDocument = Utils.getDOMDocument(new File(filePath));
return SyntaxTreeGenerator.buildTree(domDocument.getDocumentElement());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (IOException | InvalidConfigurationException e) { | |
| return new MediatorTryoutInfo(e.getMessage()); | |
| } | |
| } | |
| private STNode getSTNode(String filePath) throws IOException, InvalidConfigurationException { | |
| if (StringUtils.isEmpty(filePath)) { | |
| throw new IllegalArgumentException("FilePath is null"); | |
| } | |
| DOMDocument domDocument = Utils.getDOMDocument(new File(filePath)); | |
| return SyntaxTreeGenerator.buildTree(domDocument.getDocumentElement()); | |
| } | |
| } catch (IOException | InvalidConfigurationException e) { | |
| return new MediatorTryoutInfo(e.getMessage()); | |
| } | |
| } | |
| private STNode getSTNode(String filePath) throws IOException, InvalidConfigurationException { | |
| if (StringUtils.isEmpty(filePath)) { | |
| throw new InvalidConfigurationException("FilePath is null"); | |
| } | |
| DOMDocument domDocument = Utils.getDOMDocument(new File(filePath)); | |
| return SyntaxTreeGenerator.buildTree(domDocument.getDocumentElement()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/mediator/schema/generate/ServerLessTryoutHandler.java`
around lines 80 - 92, Update getSTNode to report an empty filePath using one of
the checked exception types already handled by handle, rather than throwing
IllegalArgumentException. Preserve the existing error message and ensure handle
returns MediatorTryoutInfo for empty paths through its current catch block.
Update OpenAPI spec generation process
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/RestApiAdmin.java (1)
127-174: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStructured error responses don't cover
APIGenExceptionswallowed deeper in the pipeline.
createAPIFromSwagger→getSynapseAPIFromSwagger→APIGenerator.generateSynapseAPIXml(). That last method catchesAPIGenExceptionand silently returnsnull(no message propagated). With the new error-reporting design here, a genuine generation failure (e.g. missing title, mandatory info) results innew GenerateAPIResponse(null, null)with no error text, rather than the descriptive error now provided forJsonProcessingException/SOAPToRESTException/etc. Consider havingcreateAPIFromSwaggercallgenerateSynapseAPI()directly (or otherwise get access to the exception message) so failures are reported consistently with the rest of this method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/RestApiAdmin.java` around lines 127 - 174, Update the Swagger generation flow in createAPIFromSwagger and getSynapseAPIFromSwagger so APIGenException from APIGenerator.generateSynapseAPIXml is not swallowed as a null result. Propagate or expose the exception message to createAPI, then return a GenerateAPIResponse containing the descriptive error text consistently with the existing JsonProcessingException handling.
🧹 Nitpick comments (2)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/APIGenerator.java (2)
223-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocal variable shadows the
versionTypefield.
ApiVersionType versionType = getNormalizedProvidedVersionType();shadows the instance fieldprivate String versionType(a different type). It's legal, but risks confusion for future edits within this method. Consider renaming the local variable (e.g.resolvedVersionType) for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/APIGenerator.java` around lines 223 - 226, Rename the local variable declared in the APIGenerator version-extraction flow from versionType to a distinct name such as resolvedVersionType, and update all references within that method while leaving the instance field versionType unchanged.
128-148: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReject unsupported
versionTypevalues
getNormalizedProvidedVersionType()returnsnullfor any unknown value, so a typo likeurllfalls back to the default context/version behavior without any signal to the caller. Accept onlycontext/url/none, and surface an explicit error for anything else.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/APIGenerator.java` around lines 128 - 148, Update getNormalizedProvidedVersionType() to accept only the supported context, url, and none values, while preserving the blank-value behavior. For any other nonblank versionType, stop returning null and surface an explicit validation error to the caller instead of falling back to default behavior; keep isProvidedVersionTypeNone() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/APIGenerator.java`:
- Around line 239-245: Update the version-suffix removal logic in APIGenerator’s
ApiVersionType.url branch so stripping a context consisting only of "/" plus
versionInPath preserves apiContext as "/". Keep the existing substring behavior
for longer contexts and retain the normalization pipeline’s leading-slash
guarantee.
---
Outside diff comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/RestApiAdmin.java`:
- Around line 127-174: Update the Swagger generation flow in
createAPIFromSwagger and getSynapseAPIFromSwagger so APIGenException from
APIGenerator.generateSynapseAPIXml is not swallowed as a null result. Propagate
or expose the exception message to createAPI, then return a GenerateAPIResponse
containing the descriptive error text consistently with the existing
JsonProcessingException handling.
---
Nitpick comments:
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/APIGenerator.java`:
- Around line 223-226: Rename the local variable declared in the APIGenerator
version-extraction flow from versionType to a distinct name such as
resolvedVersionType, and update all references within that method while leaving
the instance field versionType unchanged.
- Around line 128-148: Update getNormalizedProvidedVersionType() to accept only
the supported context, url, and none values, while preserving the blank-value
behavior. For any other nonblank versionType, stop returning null and surface an
explicit validation error to the caller instead of falling back to default
behavior; keep isProvidedVersionTypeNone() unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8ce3391-196d-49a1-a7ff-c467def67956
📒 Files selected for processing (5)
org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/APIGenerator.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/GenericApiObjectDefinition.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/RestApiAdmin.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/SwaggerConstants.javaorg.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/pojo/GenerateAPIParam.java
Comment on lines
+239
to
+245
| if (ApiVersionType.url.equals(versionType)) { | ||
| String versionInPath = StringUtils.isNotBlank(swaggerVersion) ? swaggerVersion : version; | ||
| if (StringUtils.isNotEmpty(versionInPath) && apiContext.endsWith(versionInPath)) { | ||
| // remove version from base path | ||
| apiContext = apiContext.substring(0, apiContext.length() - versionInPath.length() - 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Version-suffix stripping can drop the leading slash from apiContext.
When apiContext consists solely of "/" + versionInPath (e.g. context "/v1" with version "v1"), apiContext.substring(0, apiContext.length() - versionInPath.length() - 1) evaluates to an empty string, leaving apiContext as "" instead of "/". This produces an API context without a leading slash, which the rest of the normalization pipeline otherwise guarantees.
🐛 Proposed fix
if (ApiVersionType.url.equals(versionType)) {
String versionInPath = StringUtils.isNotBlank(swaggerVersion) ? swaggerVersion : version;
if (StringUtils.isNotEmpty(versionInPath) && apiContext.endsWith(versionInPath)) {
// remove version from base path
- apiContext = apiContext.substring(0, apiContext.length() - versionInPath.length() - 1);
+ String stripped = apiContext.substring(0, apiContext.length() - versionInPath.length() - 1);
+ apiContext = StringUtils.isBlank(stripped) ? "/" : stripped;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (ApiVersionType.url.equals(versionType)) { | |
| String versionInPath = StringUtils.isNotBlank(swaggerVersion) ? swaggerVersion : version; | |
| if (StringUtils.isNotEmpty(versionInPath) && apiContext.endsWith(versionInPath)) { | |
| // remove version from base path | |
| apiContext = apiContext.substring(0, apiContext.length() - versionInPath.length() - 1); | |
| } | |
| } | |
| if (ApiVersionType.url.equals(versionType)) { | |
| String versionInPath = StringUtils.isNotBlank(swaggerVersion) ? swaggerVersion : version; | |
| if (StringUtils.isNotEmpty(versionInPath) && apiContext.endsWith(versionInPath)) { | |
| // remove version from base path | |
| String stripped = apiContext.substring(0, apiContext.length() - versionInPath.length() - 1); | |
| apiContext = StringUtils.isBlank(stripped) ? "/" : stripped; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/customservice/synapse/api/generator/APIGenerator.java`
around lines 239 - 245, Update the version-suffix removal logic in
APIGenerator’s ApiVersionType.url branch so stripping a context consisting only
of "/" plus versionInPath preserves apiContext as "/". Keep the existing
substring behavior for longer contexts and retain the normalization pipeline’s
leading-slash guarantee.
Update LS version
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated PR to sync
stable/miintomainfollowing the successful release of v0.24.0-wso2v92.